Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type alias syntax & semantics #591

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Type alias syntax & semantics #591

wants to merge 18 commits into from

Conversation

Kronos3
Copy link
Collaborator

@Kronos3 Kronos3 commented Feb 3, 2025

Part of #113. This finishes up some changes made in #549 for syntax & semantics implementation. It adds a test to generate a struct that references a type alias.

No C++ code is generated yet for type aliases. In model elements that use type aliases, the type aliases are converted to their underlying types before generating C++ code.

@Kronos3 Kronos3 mentioned this pull request Feb 3, 2025
6 tasks
@bocchino
Copy link
Collaborator

bocchino commented Feb 4, 2025

It looks like you need to regenerate the trace info for the native build. See the instructions here: https://github.com/nasa/fpp/tree/main/compiler#building-native-binaries.

@bocchino bocchino self-requested a review February 5, 2025 17:37
Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! As discussed, let's separate out the removal of "built-in types," because removing those requires changes to F Prime. For now let's move forward with the syntax, semantics, and code generation of type aliases, without removing built-in types.

@bocchino
Copy link
Collaborator

Another issue I see is that the code generator is generating an empty TypesAc.cpp file. I think we should suppress the generation of this file. Does that require any changes to the CppDoc framework?

@Kronos3
Copy link
Collaborator Author

Kronos3 commented Feb 10, 2025

@bocchino You may need to do the trace updates again, I can't install GraalVM for Java11 since the only version available from Homebrew is jdk23 and the SDKMAN/official installation method seems to only have 17, 21, and 23.

We may also want to consider updating the Java version or putting the trace updates in CI

@Kronos3 Kronos3 requested a review from bocchino February 11, 2025 02:14
@bocchino bocchino changed the title Type alias syntax & codegen Type alias syntax & semantics Feb 11, 2025
@bocchino
Copy link
Collaborator

We may also want to consider updating the Java version or putting the trace updates in CI

Noted. The trace updates are supposed to be in CI, but they have never worked, and instead of wrangling CI I have just updated them manually. @LeStarch do you see any issue with updating the FPP Java environment from Java 11 to Java 17? We would need to do this in the GitHub actions.

Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I added some comments.

We also discussed adding a few tests for type aliases to the ScalaTest tests. Can we do that in this PR?

@bocchino
Copy link
Collaborator

I opened an issue on upgrading Java: #605.

@bocchino
Copy link
Collaborator

As discussed, I'll resolve the conflict in the native image trace file.

@bocchino
Copy link
Collaborator

I resolved the conflict. It looks like there is a test failure for fpp-to-cpp/test/struct/alias_type.fpp. A map lookup is failing.

@Kronos3
Copy link
Collaborator Author

Kronos3 commented Feb 13, 2025

I added some scala tests and found an issue in the commonType() implementation for type aliases. We now compute the "lowest common ancestor" of the type alias tree as the common type.

For example:

type T

type A1 = T
type A2 = A1
type A3 = A1

commonType(A2, A3) => A1 not the underlying type T since A1 is more descriptive. I also had to update the spec to include this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants